-
-
Notifications
You must be signed in to change notification settings - Fork 19
fix: handle property accesses cases on resolve-binding-path #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: handle property accesses cases on resolve-binding-path #218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to have handling something like :
import foo from 'node:foo'
foo.bar.functionToModify();
I think this test already covers this scenario, doesn’t it? userland-migrations/utils/src/ast-grep/resolve-binding-path.test.ts Lines 26 to 41 in 485cdc1
|
Yeah but for esm |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT ! Thanks bruno
|
||
if (propertyAccesses.length) { | ||
const pathArr = path.split("."); | ||
let newPath = ["$"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think newPath
is actually constant?
let i = 0; | ||
|
||
for (; i < propertyAccesses.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i
will actually be scoped to the parent of for
automatically, so extracting it like this is unnecessary, but perhaps you did it to make it more obvious that the parent scoping is necessary/intended?
When a module is imported with property access after require
It resolve as
bind.a.b.c
However, since the property is accessed immediately after require, it should instead resolve asbind
, because the value ofc
is directly assigned to the bind constant.This changes fixes that behavior.